agree that it's unclear the special case here is useful. either the path should be enough or we would need arbitrary ancestor / dag access from the context.

I've ended up squashing most of my work-in-progress commits together, so it's now invisible, but it might be interesting to note: I originally did comment this out entirely... but by the time I finished migrating the traversal package, it turned out so easy to keep it that I did.

So, this doesn't really argue against dropping it. But it turned out not much work (at the moment, anyway) to keep it, either.

maybe define these with the definition of LinkSystem?

one design thing this setup leaves open that is a bit worrying is that as it gets used for applied subsets of IPLD dags, we'll end up with implementations that don't fully fill out LinkContext, and will be fine for their initial use case, but will then end up in designs where tools aren't generally applicable to IPLD data because of that specificity.

I'm not super worried? The documentation does make an explicit statement that a zero-value LinkContext is to be considered acceptable. Also, that values in a LinkContext should only ever affect data availability, not actual content.

I can't enforce this programmatically, of course; that would require some kind of magic. But: that's sort of inevitable. Even if we took out LinkContext entirely, we'd still need a context.Context here... and that still has the arbitrary attachment feature. (Which is actually desired. I fully expect someone to use this to tunnel auth credentials, at some point.)

I don't really expect people to have a strong incentive to do unlawful/inadvisable things here, either.

(Although... speaking of "only data availability effects"... that does remind me that we need to get a lot more buckled down on well-typed errors throughout this whole library.)

Review wanted on naming here. (Both the functions, and the field names.)

@hannahhoward had previously commented on them being a bit of a mouthful, but rather liking that they mention "blocks" now. I think I've improved the names a bit since the draft she was looking at, but they're still a mouthful.

Probably I should s/Storage/Block/ on the field names, so they're all just consistent?

Somewhat unrelated, and perhaps a bit of a nit: I don't think func fields should be named like interfaces. It's far more natural to call lsys.ChooseEncoder(...) or lsys.EncoderChoose(...) than lsys.EncoderChooser(...). The result of that call is an encoder, not an encoder chooser :) A bit like if strings.Repeat were called strings.Repeater.

Interfaces are different, because you call methods on them, and it's good to differentiate the interface name from the method name. You just have one name here, so in my opinion it should be what best fits the method-like function calls.

FYI, I think each separate example will only render as its example func body, so the references to "store" might not show up well. You might want to use local store variables instead.

also, for the sake of keeping the example simple, I'd use &storage.Memory{} and avoid (&store).xxx

In fact, I think both examples need to be a single one; otherwise, running only the Load one will fail. This will be a problem for many scenarios like pkg.go.dev and godoc.org, where you can click "Run" on a single runnable example.

Oh, boy. Yeah. I ported this example somewhat uncritically. But you're right, it was wrong both now and before; that standalone property is really important. I'll separate these.

...although I'm not really sure how. Is there a good idiom for using the actions of one example as setup for another? If I extract a function, it disappears from the rendered example in godoc again. But the Example* function contract doesn't allow enough parameters to use it directly, either.

Maybe I wasn't clear - I mean having a single Example function, and putting all the code in there. You can't have two separate example funcs that share state. It would be a bit like having two Test funcs that share global state, but worse, since running a single Example func is their main use case for the web docs.

EncoderChooser and HasherChooser are almost always used together. Have you thought about a way to make that easier? Maybe join the two "choosers" into a single func, or maybe add a helper method to simplify calling both at once.

Yeah, that's what LinkSystem.Store(...) is 😅

The idea is that users can set these, but will almost never use them directly.

I should finish adding a documentation block to the LinkSystem declaration which describes this.

I debate maybe splitting this into a LinkSystem type that has the synthesized high level methods, and a LinkSystemConfig type that has these fields and is used to build it, but is thereafter unseen. Or just unexporting these fields and going with a constructor function. Seemed kind of heavyweight though? And then we'd end up wanting a bunch of (*LinkSystem).With{Variation}(...) LinkSystem constructor variants, too, I suspect.

Up to you, really. If Store is meant to be what most people are going to use, then SGTM.

I find it a bit weird to have to supply a storage reader and a storage writer as separate entities. When supplying both, it's going to be very common that they'll both come from the same implementation, like storage.Memory here.

So it would seem more natural to me to define an interface, and be able to supply both read and write ends in one go. For example:

store := storage.Memory{}
lsys = lsys.WithStore(store)

I'd like to try this way. It's close to what we've got already; and are several reasons I'd like to use functional interfaces and be able to set these separately:

Reason 1: It's true that when supplying both, they often come from the same implementation... but I really like not having to supply both. Making something read-only by not providing it an ability to write sits well with me. It's a "capabilities" style system, and I like those: the lines are very clear.

Reason 2: Functions as interfaces makes it easier to decorate one or the other (or both), because when they're just functions, it can be done inline. By contrast, a whole type that has methods attached to satisfy an interface has to be a top-level declaration. It's also harder to decorate just one of the several functions on an interface; doing so tends to cause one to end up with a whole suite of decorator wrapper types. I think it's been not uncommon so far for us to want to decorate one of these methods but not care much about the other.

Reason 3: An interface, because of how it has the ability to grow, makes it more easier for something to evolve to become stateful. I think that would be a bad direction of growth for these data in and out primitives. These should be really simple and really functional.

These reasons 2 and 3 may sound a little hypothetical, but I think it's what we can observe actually happening in the previous generation of interfaces for this domain: go-ipfs-blockstore grew in both these directions over time, and while the result hasn't been wrong per se, it also hasn't been very satisfying and it hasn't stayed simple; I seem to hear grumbling from people who look at that interface for long. I don't want to set ourselves up for drift in that same direction again.

Reason 4: This pattern just plain has worked well for me in the past, and I think functional interfaces are underutilized in golang :D

I'd revisit this if we did a design review in depth on the blockstore concepts, and found with confidence that we wanted a certain sequence of methods to always be available in a bundle together; or, if we have some ideas where we'd actually want to be able to do feature detection on the storage system.

At the moment, my thinking is that I don't think we'll want the ability to do further feature detection on these values in any of the places we use a LinkSystem. For example: will the traversal package ever want to check if the storage system supports GC (which is one of the other interfaces that have appeared around blockstorage)? I think it shouldn't. Things that just do loading and storing should be pretty logically unaware of anything but the read and write operations.

The best example I can think of for wanting feature detection is probably the https://pkg.go.dev/github.com/ipfs/go-ipfs-blockstore#Viewer interface, which wants to check if certain performance shortcuts are available. But, we've also seen that to be doable by checking the io.Reader returned by these functions for other features. So we probably don't need to be preparing to do things like that in two separate places.

Maybe a middle position on this -- let's try functional interfaces for now (this is also the smallest change from the current Loader and Storer system); but we stay extremely ready to change this as folks work on moving go-ipfs over to these libraries? We should get good and hard info from that process, and if we discover the need for more feature detection on storage systems during that work, then we do it. We could defer tagging a new version until we've got enough experience from that work to decide more confidently.

Somewhat unrelated, and perhaps a bit of a nit: I don't think func fields should be named like interfaces. It's far more natural to call lsys.ChooseEncoder(...) or lsys.EncoderChoose(...) than lsys.EncoderChooser(...). The result of that call is an encoder, not an encoder chooser :) A bit like if strings.Repeat were called strings.Repeater.

Interfaces are different, because you call methods on them, and it's good to differentiate the interface name from the method name. You just have one name here, so in my opinion it should be what best fits the method-like function calls.

Not sure how I feel about the simple cases having to type this out :) I get that it's the case with context.Context too, but in most cases you do end up having a ctx parameter or variable you can easily reuse. Not really the case with a custom ipld.LinkContext.

Yeah, I'd like to be able to get rid of this. I think we'd need more methods for it, though? If you have naming suggestions, I'd be happy to add them.

We can also add more methods later.

nit: could we make nil equal basicnode.Prototype.Any?

No, unfortunately :( Cycle problems would ensue :(

you declared Must methods, so you could also use them here to keep the example shorter

@hannahhoward you might wanna be aware: I didn't carry forward the byteAccessor feature detector. That might be a "yet" thing -- I'd like to try getting along without it -- but do you know if we'll be in a good place to detect a performance regression in the area that made us introduce this the first time?

I'm weakly betting it might less relevant now because the hasher doesn't force excessive copies after this rework, but, it's only a bet.

I'm going to request we put this back in before merge.

I realize the theoretical benefits in working with io.Reader data. However, in practice, 99% of practical use cases currently involve reading plain bytes in a bytes.Buffer while loading DagPB data.

The reason for this is two-fold:

• sadly the vast majority of data we work with in Filecoin and soon IPFS using go-ipld-prime is DagPB. Put simply, we're working with UnixFS v1 files (the cbor data in filecoin mostly uses cbor-gen)
• the vast majority of interfaces that feed into the io.Reader here return byte arrays. This includes go-datastore and go-ipfs-blockstore (well it returns blocks, but they have byte arrays underneath), and network messages in go-graphsync which have byte arrays at the block level. We should in the future look at the abstractions around reading data off disk -- it makes more sense that one data they would lazy load on read. But it is not that day.

This optimization is one part of a two part optimization. The other part is here -- https://github.com/ipld/go-ipld-prime-proto/blob/master/coding.go#L25. Taken together, these optimizations do not avoid extra byte array copies -- they avoid copies entirely. When working with protobuf data, which is again, unfortunately, 99% of the practical use case of this library so far, it's pretty important we don't incur an extra copy hit for using an io.Reader interface.

We detected the memory issues transferring large protobuf files in Filecoin. It's perhaps on us we didn't build effective reproducible benchmarks of this specific phenomena, but also, these complex scenarios are often. Nonetheless, it puts a lot more work to put on downstream teams detecting a likely performance regression in the name of maintaining purity of implementation on a hunch that this will fix it.

It's back in. Reintroduced with 08f13e5.

can this not be inferred from the type?

{ not ( :) it's an adapter type, not an interface cast.

i think i was wanting the apparently deprecated '-1' to indicate default length because a lot of these hashes have a single length that makes sense, and i don't want to be remembering / fumble that sha1 should be 20 bytes while sha224 should be 28 bytes every time i make one of these.

oh, sorry, I thought the comment was on the diff line, since that's what github highlighted most brightly.

Yeah, agree. I wish the go-cid and/or go-multihash libraries were more friendly about this very common user story.

I think a -1 should flow through and do whatever go-multihash does, still. And I have no idea why that's deprecated, fwiw. (A lot of things in go-multihash seem deprecated without much comment on why or what to do instead. I think some review and renovation of that is overdue.)

It's slightly on the other side of where I'm cordoning my renovation today, though.

I'm going to request we put this back in before merge.

I realize the theoretical benefits in working with io.Reader data. However, in practice, 99% of practical use cases currently involve reading plain bytes in a bytes.Buffer while loading DagPB data.

The reason for this is two-fold:

• sadly the vast majority of data we work with in Filecoin and soon IPFS using go-ipld-prime is DagPB. Put simply, we're working with UnixFS v1 files (the cbor data in filecoin mostly uses cbor-gen)
• the vast majority of interfaces that feed into the io.Reader here return byte arrays. This includes go-datastore and go-ipfs-blockstore (well it returns blocks, but they have byte arrays underneath), and network messages in go-graphsync which have byte arrays at the block level. We should in the future look at the abstractions around reading data off disk -- it makes more sense that one data they would lazy load on read. But it is not that day.

This optimization is one part of a two part optimization. The other part is here -- https://github.com/ipld/go-ipld-prime-proto/blob/master/coding.go#L25. Taken together, these optimizations do not avoid extra byte array copies -- they avoid copies entirely. When working with protobuf data, which is again, unfortunately, 99% of the practical use case of this library so far, it's pretty important we don't incur an extra copy hit for using an io.Reader interface.

We detected the memory issues transferring large protobuf files in Filecoin. It's perhaps on us we didn't build effective reproducible benchmarks of this specific phenomena, but also, these complex scenarios are often. Nonetheless, it puts a lot more work to put on downstream teams detecting a likely performance regression in the name of maintaining purity of implementation on a hunch that this will fix it.

I would like to understand more the rationale for HasherChooser and this registry, along with the code in LinkPrototype.BuildLink

It feels like a reinvention of go-multihash. How do I know every codec already registered with go-multihash is registered here? What if they are missing and I don't expect that?

Why does BuildLink take a hash sum instead of the bytes of the whole node? Instead of writing to a hash function returned by HasherChooser, why not just write to a byte buffer and then give the bytes to buildLink which can then just call cid.Prefix.Sum in the cidLink implementation?

I feel like this pushes us further into the "reinvent low level libraries" direction. While I'm open enough to the possibility of a go-cid rewrite to allow for a Link abstraction (though I'm unconvinced still about the trade-off in refactor effort, even with the link interface abstraction), I'm definitely opposed to trying to rewrite go-multihash, which has only a few dependencies. In any case, if you have Link.BuildLink take all the bytes rather than a hash, you remove all these hash library depedencies any time you want to import the codec table for example.

It is. go-multihash is frankly awful.

I covered some of this in the initial PR message, but with greater 🔥 :

go-multihash makes it very difficult to handle multihash identifiers in a forward compatible way (impossible, in fact), which is ridiculous. It has direct unconditional transitive dependencies which many projects don't need, which adds friction to adopting IPLD as a whole, and needs to be fixed. It doesn't work streamingly, which is just ridiculous. It doesn't provide a way to use a bog-standard hash.Hash without pushing patches (and a surprising number of them, because the core abstractions are terrible) directly into that library (I take part of this one back; it's just more steps than I expected, so I initially didn't see the path.), which is also ridiculous. It actively rejects multihash indicator numbers that it doesn't recognize, which makes its the opposite of future proof, and in this domain that's somewhat ridiculous. It doesn't even connect with go-cid particularly well (it predates a refactor which uses strings for immutability and map-key reasons), which is (ready?) ridiculous. There's very little that's good about the current state of go-multihash.

I did not completely remove go-multihash from the dependency tree in this diffset yet, but it's close, and I probably should, either now or soon. One of the recommendations I got from Jeromy when looking at the BuildLink method is that we could just cut out the middleman go-mulithash code, assemble the bytes ourselves, and use one of the go-cid parse methods. It's a somewhat staggering idea, but would probably honestly be more efficient than some of the dancing go-multihash does right now.

I don't doubt that this migration could be bumpy, because it does mean less things are registered by default. But I'm highly confident that getting a registry in this area somewhere is the way forward, and has to happen sometime (JS has already gone the direction of reworking these primitives so dependencies are optional, and we should too); and I think this is probably going to be one of the easier times to do it.

I'll add some more packaging to this to make the migration easier -- I'm thinking of making a go-ipld-prime/multihash/all package which, when imported, makes sure everything currently registered by go-multihash is registered here too. Any application that's a data broker that needs to support all the hashes can import that and be done. Hopefully that and a good changelog message will be enough.

(I'll confess, I didn't start out with this much aggression towards go-multicodec. It developed during the writing of this PR, when I got hands-on with it. I did not expect it to be so hard to take an interface draft using golang's standard-since-the-first-release-of-golang hash.Hash and connect it with doing hashing using a library with the word hash in the name. And it. was. awful. Hair-pulling, screaming-at-the-screen awful. Simple things should be simple. This should have been, and was not.)

It's possible that we should recuse up to renovating the go-multihash repo. However, what I came to in the course of working on this PR seems to address most of the painpoints in a way that's low impact to downstreams, other than requiring a new import statement. And there's a lot of reasons I'm happy with it:

• I don't think the upsides of dynamic registration are realizable with anything less.
• I actually got this PR done. I don't think changing go-multicodec is on the same timescale.
  • And I don't really want to couple this change to that one, because I want us to be able to start writing new code against LinkSystem now.
• If we do push these semantics back towards go-multihash later: the effect to downstreams of go-ipld-prime can be as mild as changing their import statement from go-ipld-prime/multihash/all to go-multihash/register/all.
  • Or less: the go-ipld-prime/multihash/all package can just itself be reduced to an import that passes it on, leaving downstream users will no breaking changes at all.

So: are there smells of workaround here: yes. But I think they're minimal and manageable; and the workarounds that would've been generated in this PR continuing to use go-multihash directly would actually be larger and less manageable.

The reality is there are a bunch of 3rd party hashes that end up in the current multihash registry in ipfs world and won't conform to hash.Hash

How do we expect to support that backwards compatibility?

With all the terrible glue code I just wrote. ┐_(ツ)_┌

See the new gadgets in multihash/register/sha3 for handling Shake, for example.

(Like actually, review appreciated. I just utterly invented a BlockSize which is nonsensical. My theory is that it's harmless.)

I'll admit, I had kind of a discovery process today. I didn't realize how many things in golang don't conform to hash.Hash directly.

At the same time: I think still more things conform to hash.Hash than conform to anything else, so I think sticking to that as the centroid is probably the best option available.

again - isn't the reality that we need to take existing 3rd party codecs currently being registered with mulithash, and be able to keep them working after this transition? not "write a custom shim for each case" but be backwards compatible for them? If that grows to a week+ of work, it's less likely to make it into this refactor.

Oh, "3rd party" in the sense of beyond-the-third-party-things-already-directly-depended-on-by-go-mulitcodec. Mmm. Okay, so, I think I had stated earlier that this was impossible, and I think I walk that back. On further look, it seems possible; I just didn't see how at first, because it requires multiple steps, not just the so-called register function in that library.

But I still suspect, looking over the flavor of some of the issues in that repo, and how issues and PRs seem to focus on getting new things added to the library, that the number of third-party registrations made in the wild is asymptotically close to zero.

So I think at the moment: we can functionally disregard the possibility. And if there are any applications out there which hit a speedbump here, they hit it when converting to go-ipld-prime... which is probably already in their roadmap as a speedbump, so there shouldn't be a category-change in speedbumpism, and so as a result I think everyone involved in such a hypothetical will probably survive.

Longer run: yeah, conversation is needed about how to re-reconcile this with go-multihash. I think a bigger venue than this PR is going to be needed for that, though.